Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Remove individual product from the scan carousel #1662

Merged
merged 3 commits into from
Apr 28, 2022
Merged

feat: Remove individual product from the scan carousel #1662

merged 3 commits into from
Apr 28, 2022

Conversation

bhattabhi013
Copy link
Contributor

What

Added a clear button to remove an individual product from the list of scanned products.

Video

1295.mp4

Fixes bug(s)

Part of

@bhattabhi013 bhattabhi013 requested a review from a team as a code owner April 26, 2022 19:19
@monsieurtanuki
Copy link
Contributor

@bhattabhi013 Please use recent method from DaoProductList instead of the pop you introduced:

  /// Adds or removes a barcode within a product list (depending on [include])
  ///
  /// Returns true if there was a change in the list.
  Future<bool> set(
    final ProductList productList,
    final String barcode,
    final bool include,
  )

monsieurtanuki
monsieurtanuki reviewed Apr 26, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @bhattabhi013!
Minor comments.
The main thing is to get rid of your pop method in DaoProductList, that is now redundant with a recent method (that probably didn't exist when you started).

packages/smooth_app/lib/data_models/continuous_scan_model.dart Outdated Show resolved Hide resolved
packages/smooth_app/lib/pages/product/summary_card.dart Outdated Show resolved Hide resolved
@teolemon
Copy link
Member

  • Not sure why we put the weight on the right (vs the Mockups) and thus can't put the cross on the left.
  • If in the red bar, it might send the message it will remove the red bar only.
  • Also, I expect it to be at the same distance from the right as the product name/brand are from the left.

@bhattabhi013
Copy link
Contributor Author

  • Not sure why we put the weight on the right (vs the Mockups) and thus can't put the cross on the left.
  • If in the red bar, it might send the message it will remove the red bar only.
  • Also, I expect it to be at the same distance from the right as the product name/brand is from the left.
  • I'll place the cross icon on the left.
  • I'm not sure about the second point.
  • I'll fix this and change the distance as suggested.

@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2022 •
edited
Loading

Codecov Report

Merging #1662 (4770577) into develop (2ea0da3) will decrease coverage by 0.54%.
The diff coverage is 0.37%.

@@            Coverage Diff             @@
##           develop   #1662      +/-   ##
==========================================
- Coverage     8.86%   8.31%   -0.55%     
==========================================
  Files          161     165       +4     
  Lines         6623    7116     +493     
==========================================
+ Hits           587     592       +5     
- Misses        6036    6524     +488
Impacted Files Coverage Δ
...h_app/lib/cards/category_cards/abstract_cache.dart 0.00% <0.00%> (ø)
...p/lib/cards/category_cards/asset_cache_helper.dart 0.00% <0.00%> (ø)
...p/lib/cards/category_cards/raster_async_asset.dart 0.00% <0.00%> (ø)
...oth_app/lib/cards/category_cards/raster_cache.dart 0.00% <0.00%> (ø)
..._app/lib/cards/category_cards/svg_async_asset.dart 0.00% <0.00%> (ø)
...smooth_app/lib/cards/category_cards/svg_cache.dart 0.00% <0.00%> (ø)
...t_cards/knowledge_panels/knowledge_panel_card.dart 0.00% <0.00%> (ø)
...rds/knowledge_panels/knowledge_panels_builder.dart 2.17% <ø> (ø)
...pp/lib/cards/product_cards/product_title_card.dart 0.00% <0.00%> (ø)
...cards/product_cards/smooth_product_card_found.dart 0.00% <0.00%> (ø)
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbc2aca...4770577. Read the comment docs.

@teolemon
Copy link
Member

@bhattabhi013 i was tired, cross on the right, see the original bug with mockup for reference

monsieurtanuki
monsieurtanuki approved these changes Apr 26, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @bhattabhi013!
I haven't understood where the cross icon is finally supposed to be, but as far as I'm concerned it's good!

@teolemon
Copy link
Member

Screenshot_20220426-230209.png

@bhattabhi013
Copy link
Contributor Author

Hi everyone,
Here is the screenshot of the updated UI as suggested by @teolemon.
image

@teolemon
Copy link
Member

We can harmonize the fontsize of weight perhaps, but LGTM 👍

@teolemon teolemon linked an issue Apr 27, 2022 that may be closed by this pull request
The cross needed to remove one product from the product carousel is missing #1295
Closed
@teolemon teolemon added this to the V1 milestone Apr 27, 2022
@teolemon teolemon merged commit ddc1bac into openfoodfacts:develop Apr 28, 2022
@teolemon
Copy link
Member

merged, thanks @bhattabhi013

@github-actions github-actions bot mentioned this pull request Apr 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teolemon teolemon teolemon approved these changes

@monsieurtanuki monsieurtanuki monsieurtanuki approved these changes

Assignees

@bhattabhi013 bhattabhi013

Labels
product scan carousel scan card
Projects
🤳🥫 The Open Food Facts mobile app (Android & iOS)
Archived in project
Milestone
V1
Development

Successfully merging this pull request may close these issues.

The cross needed to remove one product from the product carousel is missing
4 participants
@bhattabhi013 @monsieurtanuki @teolemon @codecov-commenter